Skip to content
This repository was archived by the owner on Jun 12, 2026. It is now read-only.

EntityTransaction.mergeFind: match by (userId, txid) when present - #152

Merged
sirdeggen merged 1 commit into
bsv-blockchain:masterfrom
b-open-io:fix/mergefind-txid-key
Apr 24, 2026
Merged

EntityTransaction.mergeFind: match by (userId, txid) when present#152
sirdeggen merged 1 commit into
bsv-blockchain:masterfrom
b-open-io:fix/mergefind-txid-key

Conversation

@shruggr

@shruggr shruggr commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Problem

EntityTransaction.mergeFind currently locates an existing row on the receiver with findTransactions({ partial: { reference, userId } }). But reference is a locally-assigned random value — each storage that first ingests a txid invents its own reference (via createAction or internalizeAction). It is not a cross-storage identifier.

If two storages independently ingest the same bitcoin transaction before sync has had a chance to propagate between them, they each assign a different reference to the same txid. Every subsequent sync between those storages then fails to recognize the rows as the same transaction: mergeFind by reference misses, mergeNew fires, and a duplicate row is inserted on the receiver.

Reproduction observed in yours-wallet

  1. User wipes local IndexedDB.
  2. syncAddresses runs against the now-empty local storage and calls internalizeAction for every BRC-29 address output found on the indexer, including inbound ordinals that the user had received long before. Each call generates a fresh local reference.
  3. User re-attaches a remote storage that already holds those same txids under its own references (from a prior session).
  4. setActive runs syncToWriter, processSyncChunk invokes mergeFind, the references don't match, every txid duplicates on the remote.

The user's ordinals then appear twice in the wallet UI.

Fix

Match by (userId, txid) when the incoming row has a txid. Fall through to (userId, reference) only for pre-broadcast rows that have not yet learned their txid — this preserves correctness for the createAction → sync-while-unsigned → signAction → sync-again flow, where the receiver's pre-broadcast row has the reference but no txid yet and needs to be matched on reference so mergeExisting can stamp the txid on it.

mergeExisting already treats reference as never-updated, so the fix leaves each storage's local reference intact; they diverge across storages, as they always did, but sync reconciles by txid and no duplicates are produced.

Scope

One function, one file. No schema changes. No other entity's mergeFind is affected.

Verification

Built against 2.1.22. Reproduced the duplication pre-fix in a live wallet, applied the fix, re-ran the same attach-remote flow end-to-end, and confirmed one row per txid on both sides with each retaining its own reference.

Prefer txid when the incoming row has one, falling through to reference
only for pre-broadcast rows. `reference` is locally-assigned by whichever
storage first ingested the row — it is not a cross-storage identifier —
so matching by reference alone duplicates rows whenever two storages
independently internalized the same txid under different local
references.

The fall-through preserves the existing behavior for the createAction →
signAction path: when a pre-broadcast row exists on the receiver under
some reference and the sender is now delivering the broadcast version
with both reference and txid present, the txid lookup misses on the
receiver's pre-broadcast row and the reference fall-through finds it so
`mergeExisting` stamps the txid on the same row instead of inserting a
twin. `mergeExisting` already treats `reference` as never-updated, so
post-fix references on each side remain whatever the local storage
assigned.

Surfaced while debugging duplicate ordinals in yours-wallet after the
user wiped local IndexedDB, let `syncAddresses` internalize BRC-29
payments into the empty local store (generating fresh local
references), then connected a remote that already held those same
txids under its own references. Connecting the remote ran a sync, the
old mergeFind by reference missed on both sides, and every ordinal
duplicated.
@sonarqubecloud

Copy link
Copy Markdown

@shruggr
shruggr marked this pull request as draft April 23, 2026 19:33
@shruggr
shruggr marked this pull request as ready for review April 24, 2026 18:17

@sirdeggen sirdeggen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me will give it an AI pass in case I missed something

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes cross-storage transaction deduplication during sync by updating EntityTransaction.mergeFind to match existing transactions by the globally stable (userId, txid) when a txid is available, falling back to (userId, reference) only when needed for pre-broadcast rows.

Changes:

  • Update EntityTransaction.mergeFind to prefer (userId, txid) lookups when ei.txid is present.
  • Add fallback lookup by (userId, reference) to support syncing into pre-broadcast rows that don’t yet have a txid.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/storage/schema/entities/EntityTransaction.ts
Comment thread src/storage/schema/entities/EntityTransaction.ts
@sirdeggen
sirdeggen merged commit 7b28f0d into bsv-blockchain:master Apr 24, 2026
8 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants